Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate delgators for all services #251

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

settermjd
Copy link
Contributor

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

ServiceManager::mergeDelegators() documentation says that it merges delegators avoiding multiple same delegators for the same service. However, from experimenting with it, duplicates are allowed. So, I've created this small patch to ensure that duplicates are removed, along with a covering test for it to verify the change.

mergeDelegators' documentation says that it merges delegators avoiding
multiple same delegators for the same service. However, from
experimenting with it, duplicates were allowed, as best as I could tell.
So, I've created this small patch to ensure that duplicates are removed,
along with a covering test for it to verify the change.

Signed-off-by: Matthew Setter <[email protected]>
@settermjd settermjd added Bug Something isn't working Revert Needed labels Dec 13, 2024
@settermjd settermjd self-assigned this Dec 13, 2024
@settermjd settermjd force-pushed the fix-bug-in-delegator-list-creation branch from 5855a09 to d734c7d Compare December 14, 2024 02:07
@settermjd settermjd requested review from gsteel and boesing December 14, 2024 02:36
Copy link
Member

@alexmerlin alexmerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the content of $this->delegators, before the fix:

array(1) {
  ["DateTime"]=>
  array(2) {
    [0]=>
    string(54) "Laminas\ContainerConfigTest\TestAsset\DelegatorFactory"
    [1]=>
    string(54) "Laminas\ContainerConfigTest\TestAsset\DelegatorFactory"
  }
}

and after the fix:

array(1) {
  ["DateTime"]=>
  array(1) {
    [0]=>
    string(54) "Laminas\ContainerConfigTest\TestAsset\DelegatorFactory"
  }
}

This fixes the duplicate delegator issue.

@settermjd settermjd merged commit 6873b9e into laminas:4.4.x Dec 19, 2024
14 checks passed
@settermjd settermjd deleted the fix-bug-in-delegator-list-creation branch December 19, 2024 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Review Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants